FED-4419 Migrate builder test to functional test, refactor gold file setup#998
FED-4419 Migrate builder test to functional test, refactor gold file setup#998btr-rmconsole-7[bot] merged 3 commits intomasterfrom
Conversation
…sions We'll follow up by adding an integration test that runs the real builder
There was a problem hiding this comment.
Pull request overview
This PR migrates the over_react builder tests from unit tests to functional tests to address incompatibilities with newer analyzer versions. The refactoring moves source files into a dedicated test package and simplifies the gold file generation process, removing workarounds that were needed to support multiple analyzer versions.
Changes:
- Migrated over_react_builder_test.dart from unit tests using build_test/build_resolvers to functional tests that run actual build_runner commands
- Removed tool/update_tests_for_dart_3.sh and related multi-analyzer-version support code
- Refactored gold file generation to use a test package at test_fixtures/test_packages/builder/ instead of copying files to a temporary directory
Reviewed changes
Copilot reviewed 26 out of 60 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/vm_tests/builder/over_react_builder_test.dart | Completely refactored to use functional tests with actual build_runner commands instead of unit tests with build_test |
| tool/update_gold_output_files.dart | Simplified to build the test package directly instead of copying files to a temporary directory |
| tool/update_tests_for_dart_3.sh | Removed as no longer needed with functional test approach |
| tool/update_tests_for_dart_3/build_test_3_updates.patch | Removed along with the script that applied it |
| test_fixtures/test_packages/builder/* | New test package structure with all source files for builder tests |
| test_fixtures/gold_output_files/* | Gold files updated with additional null safety comment lines in generated code |
| test_fixtures/gold_output_files/README.md | Simplified to remove instructions about restoring deleted files |
| .github/workflows/ci.yml | Removed calls to update_tests_for_dart_3.sh script |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Process.runSync('dart', ['pub', 'get'], workingDirectory: testPackagePath); | ||
| testPackageName = | ||
| RegExp('name: (.+)').firstMatch(File(p.join(testPackagePath, 'pubspec.yaml')).readAsStringSync())!.group(1)!; | ||
| outputDirectory = Directory.systemTemp.createTempSync().path; |
There was a problem hiding this comment.
The temporary directory created in setUpAll is never cleaned up. This will leak temporary directories on the filesystem every time tests run. Add a tearDownAll to clean up the temporary directory.
| final buildProcess = await Process.start( | ||
| 'dart', ['run', 'build_runner', 'build', '--build-filter=${p.join(tmpSourceDir.path, '**')}'], | ||
| mode: ProcessStartMode.inheritStdio); | ||
| print('Building files in test fixture ${goldsDir.path}'); |
There was a problem hiding this comment.
The print message on this line says "Building files in test fixture ${goldsDir.path}", but goldsDir is the output directory for gold files, not the test fixture directory being built. This message should probably say something like "Updating gold files in ${goldsDir.path}" or refer to sourceFixtureDir instead.
| expect(logs.where((log) => log.level >= Level.WARNING), isEmpty, | ||
| reason: 'Expected no logs at WARNING or SEVERE level, but got:\n' | ||
| '\t${logs.join('\n\t')}'); | ||
| /// Runs build_runner for a specific file, cleaning first. |
There was a problem hiding this comment.
The comment says "Runs build_runner for a specific file, cleaning first" but the function does not perform any cleaning. The clean operation only happens once in setUpAll at line 35, not before each build. Update the comment to accurately reflect that this function builds without cleaning.
| /// Runs build_runner for a specific file, cleaning first. | |
| /// Runs build_runner for a specific file without cleaning. |
| '--build-filter=lib/${sourceFilePath.replaceAll('.dart', '*.g.dart')}', | ||
| '--output=$outputDirectory' | ||
| ], | ||
| workingDirectory: testPackagePath); | ||
| } | ||
|
|
||
| Future<Null> checkBuildForFile(String assetPath, String expectedOutputAssetPath, | ||
| String pathToGoldFile) async { | ||
| final inputAsset = makeAssetId(assetPath); | ||
| String generatedOutputPath(String sourceFilePath) { | ||
| return p.join( | ||
| outputDirectory, | ||
| 'packages', | ||
| testPackageName, | ||
| sourceFilePath.replaceAll('.dart', '.over_react.g.dart'), |
There was a problem hiding this comment.
Using replaceAll to change the file extension could cause issues if the file path contains '.dart' in places other than the extension (e.g., 'my.dart.file.dart' would become 'my.over_react.g.dart.file.over_react.g.dart'). Consider using a more robust approach like removing the extension with p.withoutExtension and then appending '.over_react.g.dart', or using a regex that only matches the extension at the end of the string.
robbecker-wf
left a comment
There was a problem hiding this comment.
I like the approach of moving to functional tests! LGTM
|
QA +1 CI passes with running the new tests |
|
@Workiva/release-management-p |
Motivation
The current over_react_builder_test.dart unit test can't be authored in a way that works with both older and newer analyzer versions.
We already had some workarounds in place for some analyzer versions, added in #982
But with newer analyzer versions, there are even more incompatibilities, such as not being able to depend on build_resolvers, and continuing to maintain these tests in a single over_react version isn't feasible.
Changes
Release Notes
Review
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
Please review:
QA Checklist
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: